JSpecify: initial checks for generic type compatibility at assignments #715

NikitaAware · 2023-01-12T16:31:17Z

This pull request adds the identical type parameter nullability checks for both sides of an assignment.
for example
NullableTypeParam<@Nullable String> nullableTypeParam = new NullableTypeParam<String>(); should generate an error as the type parameters of both sides of the assignment do not have the same Nullability annotations.
This PR covers the following cases:

checks for the variable instantiation A<String> a = new A<@Nullable String>();
checks for the assignments
A<@Nullable String> t1 = new A<@Nullable String>(); A<String> t2; t2 = t1;
LHS and RHS with the exact same types
A<String> a = new A<@Nullable String>();
RHS being a subtype of LHS

class Test {
  class D<P extends @Nullable Object> {}
  class B<P extends @Nullable Object> extends D<P>{}
  void test1() {
      // BUG: Diagnostic contains: Cannot assign from type
      D<String> f = new B<@Nullable String>();
  }
}

nested generics
A<B<String>, String> a = new A<B<@Nullable String>>();

The PR does not handle pseudo-assignments due to parameter passing / returns; that support will come in a follow-up PR. Also note that this PR only changes NullAway behavior when JSpecifyMode is enabled.

msridhar · 2023-01-25T21:27:41Z

I may tweak the lambda and diamond operator test cases to be a bit clearer

This is now done. To summarize, we don't seem to crash for lambdas / method refs / diamond operators, but (unsurprisingly) we miss some bugs.

lazaroclapp

Very minor nits remaining! Looks fine to land to me once those are addressed! 🎉

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

lazaroclapp

There are two comments on the tests that should also start with // TODO. That said, other than that, this PR LGTM. @msridhar, if this ok by you, we could make the edits in the UI and land (or wait for @NikitaAware to make the edit, either works for me)!

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

lazaroclapp · 2023-02-07T17:58:19Z

Cool! Should land automatically once CI passes. Again, thanks for all the great work @NikitaAware 🎉 !

For a conditional expression _e_, the type parameter nullability annotations of the true-subpart and false-subpart should match the annotations required by _e_'s context. E.g., if _e_ is the right-hand side of an assignment, the annotations should match those for the type parameters for the assignment's left-hand side: ```java // this is fine A<@nullable String> t = condition? new A<@nullable String> : new A<@nullable String>(); // this is bad A<@nullable String> t2 = condition? new A<String> : new A<String>(); // this is also bad A<@nullable String> t1 = condition ? new A<String>(): new A<@nullable String>() ``` Other contexts include returns, parameter passing, and being nested inside another conditional expression. In our experience, it seems that `javac` captures the type parameter nullability required by _e_'s context in the type of _e_ itself. So, our handling of conditional expressions simply checks that the types of both the true and false sub-expressions of _e_ are assignable to (i.e., a subtype of) the type of _e_, using the same machinery for checking assignments from #715.

…signments (uber#715)" This reverts commit 14d3693.

msridhar and others added 30 commits September 30, 2022 08:39

an initial test case

f306db2

sample code

16654f7

add-generics-checks

7f9418b

first-case-of-generics-support

441ab7c

first-case-of-generics-support

4a9c86d

restore bad deletion

01377b6

add-generics-spport-task-1-updates

359dcc8

add-generics-spport-task-1-updates

731bf3d

add-generics-spport-task-1-updates

b9b8239

add generics check for variables

fb1beb9

Merge branch 'master' into add-generics-checks

1a23f8e

tweak error message

90d471d

more tests

98f7fa6

code cleanup.remove print statements

53ddad7

Add temporary check to consider unannoted base type

7daa52b

minor code changes

cd01ff9

add generics check for new class - check if the class is generic

202d5db

add generics check for new class - check for the type arguments

3998949

generics check for new class

fc8b99f

generics check for extended class

6425909

generics check for new class

bae84dc

generics check for new class - minor changes for error message

4aba42c

generics check for interface implementation

bca4f24

minor identation changes

99e8bf9

nullability checks for build failures

a0caac8

Add Test: generics check for subclasses

5b9d918

restructure generics tests

4cb51d9

restructure generics tests

2e4a2ef

Add tests

b1668e9

restructure code

9c8b3ae

msridhar requested a review from lazaroclapp January 25, 2023 18:57

msridhar added 5 commits January 25, 2023 11:52

move test method to more appropriate place

322c3c2

tweak method reference test

4a46a46

re-add blank line

4a1ce05

clarify method ref test

fce345b

tweak lambda and diamond operator tests

02bbd9f

msridhar added 3 commits January 25, 2023 16:16

make test Java 8 compatible

514829d

Merge branch 'master' into assigments1

2270d8d

Merge branch 'master' into assigments1

924883b

lazaroclapp reviewed Feb 6, 2023

View reviewed changes

msridhar and others added 5 commits February 6, 2023 12:39

Merge branch 'master' into assigments1

c070c22

Merge branch 'master' into assigments1

e37aa61

suggested changes

7412439

suggested changes

228cf0f

suggested changes

b7bbf7c

lazaroclapp approved these changes Feb 7, 2023

View reviewed changes

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java Show resolved Hide resolved

NikitaAware and others added 2 commits February 7, 2023 09:49

suggested changes

edb4f86

Merge branch 'master' into assigments1

18f3134

lazaroclapp enabled auto-merge (squash) February 7, 2023 17:57

lazaroclapp merged commit 14d3693 into uber:master Feb 7, 2023

This was referenced Feb 23, 2023

Handle array types when checking JSpecify assignment compatibility #740

Closed

JSpecify generics checks for conditional expressions #739

Merged

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023

Revert "JSpecify: initial checks for generic type compatibility at as…

d9db490

…signments (uber#715)" This reverts commit 14d3693.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023

Revert "JSpecify: initial checks for generic type compatibility at as…

7d8c7e4

…signments (uber#715)" This reverts commit 14d3693.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

Revert "JSpecify: initial checks for generic type compatibility at as…

0d673f3

…signments (uber#715)" This reverts commit 14d3693.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

Revert "JSpecify: initial checks for generic type compatibility at as…

cba490c

…signments (uber#715)" This reverts commit 14d3693.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

Revert "JSpecify: initial checks for generic type compatibility at as…

5d781e8

…signments (uber#715)" This reverts commit 14d3693.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSpecify: initial checks for generic type compatibility at assignments #715

JSpecify: initial checks for generic type compatibility at assignments #715

NikitaAware commented Jan 12, 2023 •

edited by msridhar

Loading

msridhar commented Jan 25, 2023

lazaroclapp left a comment

lazaroclapp left a comment

lazaroclapp commented Feb 7, 2023

JSpecify: initial checks for generic type compatibility at assignments #715

JSpecify: initial checks for generic type compatibility at assignments #715

Conversation

NikitaAware commented Jan 12, 2023 • edited by msridhar Loading

msridhar commented Jan 25, 2023

lazaroclapp left a comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

lazaroclapp commented Feb 7, 2023

NikitaAware commented Jan 12, 2023 •

edited by msridhar

Loading